Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix issue 2968 by adding a new pragma(framework) #10615

Closed
wants to merge 2 commits into from

Conversation

benjones
Copy link
Contributor

@benjones benjones commented Nov 26, 2019

Which just adds appropriate flags to the linker command.

@jacob-carlborg I sort of based this off or your old patch. LMK what I overlooked.

I copypasted stuff from pragma(lib) impl. Not sure if it's a good idea to try to unify them or not.

I added this section to json.d. I could only output it if it's not empty or something to reduce the number of tests that need to be updated?

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @benjones! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
2968 enhancement Add a pragma(framework) on osx

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#10615"

Copy link
Contributor

@jacob-carlborg jacob-carlborg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Should this pragma only be available on macOS?

  • I’m not sure if this is a good idea at all. GDC won’t support it since it doesn’t support pragma(lib)

test/runnable/test2968.d Outdated Show resolved Hide resolved
test/runnable/test2968.d Outdated Show resolved Hide resolved
assert(length == array.length);
foreach (i, x; array)
{
assert(x == cast(ulong) CFArrayGetValueAtIndex(cfa, i));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You’re storing int not ulong on the array.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am? The array is declared as ulong[5] above

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I was only looking at the array literal. My bad.

@ibuclaw
Copy link
Member

ibuclaw commented Nov 26, 2019

  • I’m not sure if this is a good idea at all. GDC won’t support it since it doesn’t support pragma(lib)

Furthermore, even if GDC did support it, there are still shortcomings to the pragma() approach that means it still won't work if you are linking to a project/package that has pragma(framework).

@benjones
Copy link
Contributor Author

Thanks for the feedback. If people decide that this isn't a good idea, then we should probably close the bug report wontfix.

Is pragma(lib) useful in gdc? It looks like only when the libs can be stored in the actual object file, but I don't know which formats actually support that.

@adamdruppe seemed enthusiastic about this before... do you still feel that way?

@adamdruppe
Copy link
Contributor

Yes, I am still for it. Just because it doesn't work everywhere - even if it doesn't work most places - doesn't mean it isn't useful where it does work, and it still serves as formal, machine-readable documentation in the cases where it doesn't work.

@jacob-carlborg
Copy link
Contributor

Needs a PR for the spec as well: https://dlang.org/spec/pragma.html.

@benjones
Copy link
Contributor Author

Another note, since this writes the framework to the moduleDeps file, the framework info could be picked up by a build system tool

@kinke
Copy link
Contributor

kinke commented Nov 29, 2019

Furthermore, even if GDC did support it, there are still shortcomings to the pragma() approach that means it still won't work if you are linking to a project/package that has pragma(framework).

+1. Extending the weak pragma(lib) (on Posix) approach would be a bad idea IMO. Linker directives make sense if they can be embedded in the object file (pragma(lib) and pragma(linkerDirective) on Windows), but can be easily misleading otherwise (need to import that .d source file in the compiler invocation performing the linking to have an effect). [And I'd find framework way too generic.]

@adamdruppe
Copy link
Contributor

Just put a note in the documentation saying the result is implementation specific.

Don't let the perfect be the enemy of the good.

@Geod24
Copy link
Member

Geod24 commented Dec 2, 2019

I'm also concerned about this.
pragma(lib) sounded like a good idea, but in practice didn't pan out so well. pragma(framework) limitations are already quite obvious. And willingly putting something that is implementation-defined is not solving the problem, it's ignoring it. And I don't see it enabling something that wasn't already possible, so IMO we should just mark this issue as WONTFIX.

@adamdruppe
Copy link
Contributor

adamdruppe commented Dec 2, 2019 via email

@WalterBright
Copy link
Member

I'll have to defer to those expert on the MacOS as to whether it is useful or not. Do other languages on the Mac support such a mechanism?

@jacob-carlborg
Copy link
Contributor

jacob-carlborg commented Dec 5, 2019

Do other languages on the Mac support such a mechanism?

Kind of. Objective-C and Swift supports autolinking of frameworks. Basically you only need to import the framework and it will automatically be linked when building.

I think Clang implements this using LC_LINKER_OPTION load commands. Can GCC output these?

@jacob-carlborg
Copy link
Contributor

jacob-carlborg commented Dec 5, 2019

Extending the weak pragma(lib) (on Posix) approach would be a bad idea IMO. Linker directives make sense if they can be embedded in the object file (pragma(lib) and pragma(linkerDirective) on Windows)

@kinke Linker directives can be embedded in Mach-O files, see my previous comment: #10615 (comment).

@kinke
Copy link
Contributor

kinke commented Dec 5, 2019

Well if it can in a suitable manner (embedded strings as additional linker cmdline arguments?), then I think it's a no-brainer to extend existing pragma(linkerDirective) to support these frameworks.

@kinke
Copy link
Contributor

kinke commented Dec 5, 2019

[Btw, LLD 9 added support for embedded library dependencies in ELF object files, suitable for pragma(lib), see https://github.com/ldc-developers/ldc/issues/3245.]

@jacob-carlborg
Copy link
Contributor

jacob-carlborg commented Dec 5, 2019

Well if it can in a suitable manner (embedded strings as additional linker cmdline arguments?), then I think it's a no-brainer to extend existing pragma(linkerDirective) to support these frameworks.

As far as I can see it will embed strings as additional linker command line arguments. Here's an example of one of the load commands from the object dumper:

Load command 15
     cmd LC_LINKER_OPTION
 cmdsize 40
   count 2
  string #1 -framework
  string #2 CoreFoundation

To me, that looks very much like a linker command line argument. The comment in the header file says:

The linker_option_command contains linker options embedded in object files.

@kinke
Copy link
Contributor

kinke commented Dec 5, 2019

Okay perfect, that's analogous to MS COFF linker directives then. Then I'd propose either pragma(linkerDirective, "-framework", "CoreFoundation"), or pragma(linkerDirective, "-framework CoreFoundation") with the compiler tokenizing the string into possibly multiple linker flags (I prefer the first explicit variant).

@benjones
Copy link
Contributor Author

benjones commented Dec 5, 2019

Do you think there's an advantage to having the user write out the whole linker command rather than using the proposed syntax? If we use linkerDirective, the user would have to static if it out on "not macos" builds, whereas pragma(framework) might only be examined on macos. I'll defer to the experts, but I kind of like the pragma(framework) syntax.

@kinke
Copy link
Contributor

kinke commented Dec 5, 2019

Yes, I absolutely prefer having one generic way of expressing the same thing (linker flags embedded in object files) for multiple targets over a ever-growing family of target- and compiler-specific pragmas.

@adamdruppe
Copy link
Contributor

I can live with linkerDirective instead of pragma framework if it actually works.

@benjones
Copy link
Contributor Author

benjones commented Dec 5, 2019

OK, I'll look into that approach. Is there already code in the machobj backend for LC_LINKER_OPTION stuff or will that need to be added?

@jacob-carlborg
Copy link
Contributor

OK, I'll look into that approach. Is there already code in the machobj backend for LC_LINKER_OPTION stuff or will that need to be added?

No, that needs to be added. You can look at one of my pull requests how to add a new load command: #10476.

@ibuclaw
Copy link
Member

ibuclaw commented Dec 9, 2019

Well if it can in a suitable manner (embedded strings as additional linker cmdline arguments?), then I think it's a no-brainer to extend existing pragma(linkerDirective) to support these frameworks.

I had to do a double take, apparently I completely missed the introduction of that pragma.

@ibuclaw
Copy link
Member

ibuclaw commented Dec 9, 2019

[Btw, LLD 9 added support for embedded library dependencies in ELF object files, suitable for pragma(lib), see https://github.com/ldc-developers/ldc/issues/3245.]

I wrote such a thing for gdc too, which writes the data into a custom section, then all objects are scanned by the driver.

From what I remember, it didn't extend to linking against D libraries (neither static nor dynamic) that may have this section embedded in. As the linked patch for the LLVM linker, I assume you won't have that problem?

@kinke
Copy link
Contributor

kinke commented Dec 9, 2019

At least I don't plan to do any scanning of objects/libraries in the LDC driver; for Posix targets, we simply invoke gcc or clang for linking, so the plan is to let them take care of everything, even if that means that this special section might only work in combination with clang and LLD for now.

@kinke
Copy link
Contributor

kinke commented Dec 17, 2019

This is implemented for LDC now in ldc-developers/ldc#3259. It includes a small frontend patch for pragma(linkerDirective) to

  • be analyzed for all targets, not just MS COFF
  • support multiple string arguments, not just 1

=> pragma(linkerDirective, "-framework", "CoreFoundation");

@benjones benjones closed this Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants